feat(reconcile): add a Webhook kind#1772
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds webhook endpoint validation and uniqueness checks, Connect error classification, desired-state reconciliation and export support, CLI registration, and documentation for webhook semantics. ChangesWebhook reconciliation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report for CI Build 29985960599Warning No base build found for commit Coverage: 46.598%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
71b308b to
45fd480
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/api/v1beta1connect/webhook.go (1)
15-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
webhookErrCodeisn't applied toListWebhooks/DeleteWebhook.
CreateWebhookandUpdateWebhooknow surface proper status codes viawebhookErrCode, butListWebhooks(line 88) andDeleteWebhook(line 108) still hardcodeconnect.CodeInternal. A delete of a nonexistent webhook would plausibly returnwebhook.ErrNotFoundfrom the service and should map toCodeNotFound, matching this handler's own new convention.♻️ Apply consistently
err := h.webhookService.DeleteEndpoint(ctx, webhookID) if err != nil { - return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("DeleteWebhook: webhook_id=%s: %w", webhookID, err)) + return nil, connect.NewError(webhookErrCode(err), fmt.Errorf("DeleteWebhook: webhook_id=%s: %w", webhookID, err)) }internal/reconcile/webhook.go (1)
81-95: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExtract a shared normalize+validate+dedupe step; also trim URLs to match server normalization.
validateWebhookSpectreats a whitespace-only URL as empty but then parses/stores/compares the raw untrimmed value, whilecore/webhook/service.go'sCreateEndpoint/UpdateEndpointalwaysstrings.TrimSpacethe URL before validating/storing. A spec URL with incidental whitespace can pass client-side validation yet fail to match the server's (trimmed) URL indiffWebhooks'sbyURLlookup, producing a spurious "add" that the server then rejects as a duplicate. Separately, the duplicate-URL "seen"-map dedupe loop is written independently in bothdiffWebhooksandValidate.
internal/reconcile/webhook.go#L81-L95: trims.URL(e.g.s.URL = strings.TrimSpace(s.URL)) before parsing/validating, and use the trimmed value for storage/comparison indiffWebhooks.internal/reconcile/webhook_reconciler.go#L37-L53: replace the standalone validate+seen-map loop with a shared helper (e.g.normalizeAndValidateWebhookSpecs([]WebhookSpec) ([]WebhookSpec, error)) reused bydiffWebhooks, so both call sites trim/validate/dedupe identically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5cbe9a70-6de0-457b-bfd2-93c50daea78f
📒 Files selected for processing (11)
cmd/reconcile.gocore/webhook/service.gocore/webhook/service_test.godocs/content/docs/reconcile.mdxdocs/content/docs/reference/cli.mdxinternal/api/v1beta1connect/webhook.gointernal/reconcile/role.gointernal/reconcile/webhook.gointernal/reconcile/webhook_reconciler.gointernal/reconcile/webhook_reconciler_test.gointernal/reconcile/webhook_test.go
| endpoint.URL = strings.TrimSpace(endpoint.URL) | ||
| if err := validateEndpoint(endpoint); err != nil { | ||
| return Endpoint{}, err | ||
| } | ||
| if err := s.ensureURLIsFree(ctx, endpoint.URL, endpoint.ID); err != nil { | ||
| return Endpoint{}, err | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
TOCTOU race on URL uniqueness, plus a full table scan on every write.
ensureURLIsFree lists all endpoints and checks in Go, then Create/UpdateByID runs as a separate, unsynchronized step. Two concurrent CreateEndpoint calls for the same URL can both pass the check and both persist — exactly the "two endpoints sharing a url" scenario that internal/reconcile/webhook.go's diffWebhooks has to special-case as an unrecoverable error ("the url identity is ambiguous... delete the extra one by hand"). This check-then-act pattern is unreliable without a DB-level unique constraint or a lock/transaction spanning the check and the write.
Separately, s.eRepo.List(ctx, EndpointFilter{}) fetches every endpoint on every create/update, which won't scale as the number of webhooks grows.
Recommend enforcing uniqueness with a unique index at the storage layer (with the app-level check kept as a fast-path/better error message), and, if a targeted lookup by URL is available on the repository, using that instead of a full list scan.
Also applies to: 77-83, 108-123
There was a problem hiding this comment.
Deferring this on purpose, tracked under "Not in this PR" in the description. The real fix is a DB unique index on webhook_endpoints.url, which is a migration with prod-data implications, so it lands separately. Until then the reconcile diff fails loudly and names the duplicate ids to clean up, and the app-level check stays as a fast path with a clear error. The full-list scan on each write goes away with that same follow-up (a lookup by url).
There was a problem hiding this comment.
@rohilsurana, acknowledged. Deferring the database-level uniqueness constraint is reasonable given the migration and production-data implications. The current application-level guard plus reconcile-time duplicate detection provides a clear interim failure mode.
Would you like me to open a GitHub follow-up issue for the unique index and targeted URL lookup?
✏️ Learnings added
Learnt from: rohilsurana
Repo: raystack/frontier PR: 1772
File: core/webhook/service.go:53-59
Timestamp: 2026-07-23T06:48:13.000Z
Learning: For `core/webhook/service.go`, uniqueness of `webhook_endpoints.url` is intentionally deferred to a separate production-data-aware migration that adds a database unique index. Until then, the service-level `ensureURLIsFree` full-list check is an interim fast-path, and webhook reconciliation detects duplicate URL identities and reports the duplicate endpoint IDs for manual cleanup. A targeted repository lookup by URL is planned with that follow-up.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/webhook/service.go (1)
53-59: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCanonicalize URLs before enforcing identity uniqueness.
Only surrounding whitespace is normalized here, while
ensureURLIsFreecompares strings byte-for-byte. Equivalent targets such ashttps://EXAMPLE.comandhttps://example.com/can therefore create multiple records for one webhook destination. Parse and canonicalize the URL once, persist that canonical form, and compare canonical values.Also applies to: 77-83
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e3aa9d73-003c-438e-8cbc-29f324acdb9c
📒 Files selected for processing (6)
core/webhook/service.gocore/webhook/service_test.gointernal/api/v1beta1connect/webhook.gointernal/reconcile/webhook.gointernal/reconcile/webhook_reconciler.gointernal/reconcile/webhook_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- core/webhook/service_test.go
- internal/reconcile/webhook.go
- internal/reconcile/webhook_reconciler.go
- internal/reconcile/webhook_test.go
Webhook description and state used keep-if-omitted: an omitted field kept the server value, so you could not clear a description or reset a disabled endpoint by leaving the field out. That breaks rule 2's one field model. resolve() now lays the file's present fields over the defaults (description empty, state enabled), so an omitted field converges to its default and nothing is merged from the server. Export already drops default-valued fields, so the round-trip still plans zero ops.
d2832dc to
e7a4043
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0fc7b71-abab-40ca-b59e-084866d9f105
📒 Files selected for processing (11)
cmd/reconcile.gocore/webhook/service.gocore/webhook/service_test.godocs/content/docs/reconcile.mdxdocs/content/docs/reference/cli.mdxinternal/api/v1beta1connect/webhook.gointernal/reconcile/role.gointernal/reconcile/webhook.gointernal/reconcile/webhook_reconciler.gointernal/reconcile/webhook_reconciler_test.gointernal/reconcile/webhook_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/reconcile/role.go
- core/webhook/service_test.go
- internal/api/v1beta1connect/webhook.go
- docs/content/docs/reference/cli.mdx
- internal/reconcile/webhook.go
- internal/reconcile/webhook_reconciler_test.go
- core/webhook/service.go
- internal/reconcile/webhook_reconciler.go
…nverging The reconcile guide still said description and state keep their server value when omitted, which the diff no longer does. Correct it to the one field model: an omitted field converges to its default (description empty, state enabled), and note export omits default-valued fields so the round-trip plans nothing.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 29b1521d-bfb2-4366-948c-21bffd0dfc70
📒 Files selected for processing (1)
docs/content/docs/reconcile.mdx
url.Parse + IsAbs + a scheme check let a hostless url like https:// or http:///path through: it is absolute and uses http(s), but has no host to deliver to. Reject an empty host in both the reconcile plan-time check and the server-side validateEndpoint, so the two stay in sync and the round-trip holds.
…point Say the url must be a valid absolute HTTP(S) url, and replace the vague "one that is missing" with "an endpoint that is missing", per review.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ad3942bb-806d-422b-8067-4925d028b224
📒 Files selected for processing (5)
core/webhook/service.gocore/webhook/service_test.godocs/content/docs/reconcile.mdxinternal/reconcile/webhook.gointernal/reconcile/webhook_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- core/webhook/service_test.go
- core/webhook/service.go
- internal/reconcile/webhook.go
- internal/reconcile/webhook_test.go
| - The URL must be a valid absolute HTTP(S) URL, and it is the identity. If two endpoints on the | ||
| server share a URL, the identity is ambiguous: the plan fails and names the ids so you can | ||
| remove the extra one by hand. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document URL trimming in the identity rule.
The reconciliation contract identifies webhooks by trimmed URLs, but this section only says that the URL is the identity. State explicitly that surrounding whitespace is trimmed before validation and identity comparison.
Proposed wording
-- The URL must be a valid absolute HTTP(S) URL, and it is the identity.
+- The URL is trimmed before validation and identity comparison, and must be a valid absolute HTTP(S) URL.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - The URL must be a valid absolute HTTP(S) URL, and it is the identity. If two endpoints on the | |
| server share a URL, the identity is ambiguous: the plan fails and names the ids so you can | |
| remove the extra one by hand. | |
| - The URL is trimmed before validation and identity comparison, and must be a valid absolute HTTP(S) URL. If two endpoints on the | |
| server share a URL, the identity is ambiguous: the plan fails and names the ids so you can | |
| remove the extra one by hand. |
2 participants
What
Adds a
Webhookkind to the reconcile flow, so webhook endpoints are managed from adesired-state file like platform users, permissions, roles, and preferences. Also adds the
server-side validation that lets the kind round-trip cleanly.
How the kind behaves
Follows the rules in RFC 0001. A webhook is an object keyed by its URL.
needs
delete: true.value under the one field model: a field written in the file is used as-is, and an omitted
field takes its default, nothing is merged from the server.
subscribed_eventsis the full desired set: an empty or omitted list means all events (theserver default), deduplicated and compared as a set.
descriptiondefaults to empty, so omitting it clears any description on the server.statedefaults toenabled, the state the server gives a new endpoint, so omitting itconverges the endpoint to enabled.
on read, so it is never in the file, a plan, or an export.
Files from
frontier exportreflect the current state and leave out any field already at itsdefault (an
enabledstate, an empty description), so reconciling an export plans nothing andthe normal edit-the-export workflow is unaffected. Only a hand-trimmed file that drops a
non-default field sees it reset to the default (dropping a
disabledstate re-enables theendpoint, dropping a description clears it). This mirrors the Role kind's field model from
#1779.
Server-side validation (closes the round-trip gap)
The reconcile flow treats the URL as an endpoint's identity and manages state as
enabled/disabled. For export to round-trip over every reachable server state, the server must
not hold values the reconciler cannot represent. So the webhook service now validates on create
and update:
enabledordisabled(or empty, which defaults toenabled),The URL is trimmed before it is validated, stored, and compared, so the server and the
reconciler agree on the same value. Invalid input returns
InvalidArgument, and a duplicateURL returns
AlreadyExists. The handler maps webhook errors to status codes across create,update, list, and delete, so for example updating a missing webhook reads as
NotFoundinstead of a generic internal error. (Delete stays idempotent: removing an endpoint that is
already gone reports success, which is what the reconcile flow wants.)
Uniqueness is enforced in the service. A DB unique index on
webhook_endpoints.urlwould be astronger guarantee (the service check is a best-effort fast path with a clear error, and has a
narrow check-then-write race); that index is a follow-up, since it is a migration with
prod-data implications. Either way, a server that already holds duplicate or malformed rows
from before this change would need a one-time cleanup.
Changes
internal/reconcile/webhook.go,webhook_reconciler.go: the kind, its diff (each fieldresolved to the whole desired value over its default), export, the per-document
Validate,and one shared step that trims, validates, and dedupes the URLs.
cmd/reconcile.go: register the kind and update the help text.core/webhook/service.go: validate the URL (absolute, http/https), trim it, validate state,enforce URL uniqueness, with a service test.
internal/api/v1beta1connect/webhook.go: map validation errors toInvalidArgument/AlreadyExists/NotFoundacross create, update, list, and delete.Testing
go test ./internal/reconcile/... ./core/webhook/...passes, including the diff, apply,export round-trip, dedup, URL trimming, the http(s) scheme check, the description and state
default-converging cases, and the server-side validation cases.
go build,go vet,gofmt, andgolangci-lintare clean.Not in this PR
Two SSRF and uniqueness hardening items from review are deferred on purpose:
hard constraint. It is a migration with prod-data implications, so it is a separate change.
pre-existing delivery path and is a security change of its own, best done in a dedicated PR.
This PR does restrict the URL scheme to http(s).